-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[BWC/BWS] Add TSS to BWC & BWS #3895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v11
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A roundtrip failure test may be good - add shortTag to either the encrypt or decrypt, but not both.
Truthiness tests may be good. What's the expected behavior if these flags are "true"? "false"?
* @param {string} [opts.wallets.copayerId] | ||
* @param {string} [opts.wallets.copayerId.tokenAddress] | ||
* @param {string} [opts.wallets.copayerId.multisigContractAddress] | ||
* @param {function} cb Callback function in the standard form (err, results) | ||
* @returns | ||
*/ | ||
getStatusAll(credentials, opts, cb) { | ||
getStatusAll(credentials: Array<Credentials>, opts, cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, we should remove jsdocs from ts files and replace them with typed params
@@ -100,8 +107,22 @@ export class Credentials { | |||
* @param {string} opts.requestPrivKey | |||
* @returns | |||
*/ | |||
static fromDerivedKey(opts) { | |||
$.shouldBeString(opts.coin); | |||
static fromDerivedKey(opts: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params are typed - rm jsocs (except description)
zh: Mnemonic.Words.CHINESE, | ||
fr: Mnemonic.Words.FRENCH, | ||
it: Mnemonic.Words.ITALIAN | ||
const wordsForLang = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative:
`
// Define supported languages with type safety
const languages = ['en', 'es', 'ja', 'zh', 'fr', 'it'] as const;
type Language = typeof languages[number]; // Creates type literal: 'en' | 'es' | 'ja' | 'zh' | 'fr' | 'it'
const wordsForLang: Record<Language, Array> = {
en: Mnemonic.Words.ENGLISH,
es: Mnemonic.Words.SPANISH,
ja: Mnemonic.Words.JAPANESE,
zh: Mnemonic.Words.CHINESE,
fr: Mnemonic.Words.FRENCH,
it: Mnemonic.Words.ITALIAN,
};
`
Then in your KeyOptions interface, language?: Language
@@ -51,8 +53,8 @@ export interface KeyOptions { | |||
useLegacyPurpose?: boolean; | |||
useLegacyCoinType?: boolean; | |||
nonCompliantDerivation?: boolean; | |||
language?: string; | |||
algo?: string; // eddsa or ecdsa (Bitcoin) by default | |||
language?: keyof typeof wordsForLang; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. If you don't want to make that refactor, consider uniformity of string literal definitions - you define the KeyAlgorithm string literal above with keyof typeof
, but Language (analog) is defined on the itnerface directly
* @param {String} opts.password encrypting password | ||
* @param {String} seedType new|extendedPrivateKey|object|mnemonic | ||
* @param {String} seedData | ||
* @param {KeyOptions} opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm jsdocs
const { value, algo } = params; | ||
switch (String(algo).toUpperCase()) { | ||
switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optional chaining after toUpperCase is extraneous
const { value, algo } = params; | ||
switch (String(algo).toUpperCase()) { | ||
switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous optional chaining after toUpperCase
switch (String(params?.algo).toUpperCase()) { | ||
#getPrivKey(params: { algo?: KeyAlgorithm; } = {}) { | ||
const { algo } = params; | ||
switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous optional chaining after upperCase()
switch (String(params?.algo).toUpperCase()) { | ||
#getPrivKeyEncrypted(params: { algo?: KeyAlgorithm; } = {}) { | ||
const { algo } = params; | ||
switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous optional chaining after upperCase()
switch (String(params?.algo).toUpperCase()) { | ||
#getFingerprint(params: { algo?: KeyAlgorithm; } = {}) { | ||
const { algo } = params; | ||
switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous optional chaining after upperCase()
supportStaffWalletId: any; | ||
timeout: any; | ||
r: Request; | ||
credentials: CredT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could lead to type errors since it's initialized as null.
@@ -59,13 +68,13 @@ export class Request { | |||
useSession?: boolean | |||
) { | |||
if (this.credentials) { | |||
headers['x-identity'] = this.credentials.copayerId; | |||
headers['x-identity'] = (this.credentials as Credentials).copayerId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be ensuring that this.credentials is a Credentials, and not some other 'CredT'?
const publicKey = this.keychain.commonKeyChain.substring(0, 66); | ||
const chainCode = this.keychain.commonKeyChain.substring(66); | ||
const c = super.createCredentials(password, opts); | ||
const xPubKey = new BitcoreLib.HDPublicKey({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one of the subject to change parts? xPubKey unused (use commented below)
/** | ||
* Threshold Signature Scheme (TSS) client class | ||
* @param {ITssKeyGenConstructorParams} params Constructor parameters | ||
* @param {EventEmitterOptions} eventOpts Options object for EventEmitter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unresolved
$.checkArgument(params.key instanceof Key, 'key must be an instance of Key'); | ||
|
||
this.#request = new Request(params.baseUrl, { | ||
r: params.request, // For testing only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving this in here?
const prevRound = thisRound - 1; // Get previous round's messages | ||
const { body } = await this.#request.get(`/v1/tss/keygen/${this.id}/${prevRound}`) as RequestResponse; | ||
|
||
if (body.messages?.length === this.n - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a clarifying comment above this conditional
* Get the key object if the key generation process is complete | ||
* @returns {TssKey|null} The keychain object if the key generation process is complete, otherwise null | ||
*/ | ||
getKeyChain(): TssKey | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it seem like !this.#keygen≥isKeyChainReady() is the primary flow. If that's not the case, consider:
if (!this.#kygen.isKeyChainReady()) { return null };
... primary flow & return key
}, | ||
metadata: { | ||
id: string; | ||
m: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n is number of participants, and m is number of required signers - right? Brief inline documentation in this file would be good, especially for ambiguous fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment in TssKeyGen class below
}): string { | ||
const { partyId, partyPubKey, opts } = params; | ||
const extra = params.extra || ''; | ||
const data = [this.id, partyId, this.m, this.n, extra].join(':'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the default value of extra (or if it's undefined), then you end up with a ':' at the end of your string.
const myParams = {
id: '123',
partyId: '456',
m: 3,
n: 5
};
const myStr = [myParams.id, myParams.partyId, myParams.m, myParams.n, myParams.extra].join(':');
const myStr2 = [myParams.id, myParams.partyId, myParams.m, myParams.n, ''].join(':');
console.log(myStr);
console.log(myStr2);
If that's not the intended behavior -
Suggestion:
const myJoinCodeArr = [this.id, partyId, this.m, this.n];
if (params.extra) { myJoinCodeArr.push(params.extra };
const code = myJoinCodeArr.join(':');
privateKey: this.#requestPrivateKey, | ||
opts | ||
}); | ||
return code.toString(opts?.encoding || 'hex'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If opts.encoding, then is it ensured to be in your list of provided values? Would validation be useful here?
const encodingSupported = ['hex', 'base64', 'utf8', 'binary'].includes(opts?.encoding);
const encoding = encodingSupported ? opts.encoding : 'hex';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, as an args check?
$.checkArgument(code, 'Missing required param: code'); | ||
$.checkArgument(typeof code === 'string' || Buffer.isBuffer(code), '`code` must be a string or buffer'); | ||
|
||
code = Buffer.isBuffer(code) ? code : Buffer.from(code, opts?.encoding || 'hex'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see note in createJoinCode comment above
this.partyId = parseInt(partyId); | ||
|
||
const msg = await keygen.initJoin(); | ||
password = password || extra; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen if password isn't provided in opts, and there's no 'more'?
It will be an empty string on the reassign - is that alright?
|
||
/** | ||
* Export the session for storage | ||
* @returns {string} Session string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string format would be nice: 'id:partyId:m:n:keygenSession'
And in the restoreSession below it.
/** | ||
* Threshold Signature Scheme (TSS) client class | ||
* @param {ITssConstructorParams} params Constructor parameters | ||
* @param {EventEmitterOptions} eventOpts Options object for EventEmitter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unresolved
const prevRound = thisRound - 1; // Get previous round's messages | ||
const { body } = await this.#request.get(`/v1/tss/sign/${this.id}/${prevRound}`) as RequestResponse; | ||
|
||
if (body.messages?.length === this.#tssKey.metadata.m - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be really useful here
This PR:
Note: I tried to be careful NOT to import the TSS classes into any existing files. The reason for this is so that it doesn't break any frontend webpack/vite/etc compilations with the underlying TSS WASM dependencies...which might be a pain to configure.